feat: expose narrow setters on EventSource for RETRY-spec regime switching - #109
Draft
tanderson-ld wants to merge 2 commits into
Draft
feat: expose narrow setters on EventSource for RETRY-spec regime switching#109tanderson-ld wants to merge 2 commits into
tanderson-ld wants to merge 2 commits into
Conversation
… EventSource Adds two narrow public setter methods on EventSource for SDK-driven regime switching. Motivating use case is RETRY-spec conformance in server SDKs: on classification of a failure as "unexpected" (per RETRY §1.6 / §1.7), the SDK's data source needs to shift the retry timing into an extended regime (e.g. initial 5 min, max 1 hr), and shift back after healthy operation. Cross-referenced in launchdarkly/sdk-scratchpad's server-sdk-guide.md, and tracked as SDK-2789 (Java) under the RETRY-conformance epic SDK-2775. API: - setInitialRetryDelayMillis(long) updates the existing volatile baseRetryDelayMillis field (the same field wire-side SetRetryDelayEvent already updates). If the current strategy is a DefaultRetryDelayStrategy, the exponent counter is also reset so the first subsequent apply() uses the new base directly. Non-Default strategies just see the new base on the next apply() call. - setMaxRetryDelayMillis(long) constructs a new DefaultRetryDelayStrategy with the specified max delay and the exponent counter reset to 0, preserving the current backoff multiplier and jitter multiplier, and atomically swaps the reference. No-op for custom strategy impls (which don't expose a max-delay concept via the abstract interface). Both setters realize the "reset n when delays change" invariant from the LaunchDarkly server-SDK implementation guide: the first attempt in a new regime uses the new initial delay directly rather than newBase * 2^oldN. Under the hood: DefaultRetryDelayStrategy gains two package-private helpers (withResetCounter and withMaxDelayMillisAndResetCounter) that build copies with a fresh counter. The public builder methods (maxDelay, backoffMultiplier, jitterMultiplier) are unchanged. currentRetryDelayStrategy is now volatile to support the "caller can invoke setters from any thread" contract. Tests: 6 new tests covering direct setter effects, counter-reset behavior, composed extended-regime sequence, wire-hint interaction, and no-op behavior on non-Default strategies. Full unit suite and sse-contract-tests both green.
An SDK error handler running under ErrorStrategy.alwaysContinue calls setInitialRetryDelayMillis / setMaxRetryDelayMillis inside its handleError callback, in response to a fault that has just been classified as UNEXPECTED per the RETRY specification. The immediately-prior computeReconnectDelay() call had already stored nextReconnectDelayMillis using the pre-transition strategy, so without a recompute the upcoming reconnect would use the OLD regime's timing (e.g., 1 ms normal-regime delay) and the extended-regime backoff would kick in only starting from the NEXT fault. Fix: - Track a pendingReconnectWait flag: set by computeReconnectDelay after a fault, cleared by tryStart on successful reconnect. - setInitialRetryDelayMillis and setMaxRetryDelayMillis, if pendingReconnectWait is true, recompute nextReconnectDelayMillis with the just-updated strategy. Do NOT advance the strategy (prior computeReconnectDelay already did that; advancing here would double-increment the counter for the next fault). Verified via the sdk-test-harness RETRY-conformance streaming/retry test "enters extended-regime backoff after unexpected HTTP error", which is now green (was failing prior to this fix because the SDK reconnected at normal-regime timing after the first 401).
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds two narrow public setter methods on
EventSource—setInitialRetryDelayMillis(long)andsetMaxRetryDelayMillis(long)— for SDK-driven regime switching. The motivating use case is RETRY-spec conformance in server SDKs: on classification of a failure as "unexpected" (per RETRY §1.6 / §1.7), the SDK's data source shifts retry timing into an extended regime (e.g., initial 5 min, max 1 hr), and shifts back after healthy operation.What ships
Two public methods on
EventSource:setInitialRetryDelayMillis(long millis)— updates the existing volatilebaseRetryDelayMillisfield (same field the wire-sideSetRetryDelayEventalready updates); if the current strategy is aDefaultRetryDelayStrategy, the exponent counter is reset so the first subsequentapply()uses the new base directly.setMaxRetryDelayMillis(long millis)— constructs a new immutableDefaultRetryDelayStrategywith the specified max delay and the exponent counter reset to 0, preserving the current backoff multiplier and jitter multiplier, and atomically swaps the reference. No-op for custom strategy impls (which don't expose a max-delay concept via the abstract interface).Both realize the "reset
nwhen delays change" invariant from the LaunchDarkly server-SDK implementation guide: the first attempt in a new regime uses the new initial delay directly rather thannewBase × 2^oldN.Two package-private helpers on
DefaultRetryDelayStrategy:withResetCounter()andwithMaxDelayMillisAndResetCounter(long). Existing public builder methods (maxDelay,backoffMultiplier,jitterMultiplier) are unchanged.currentRetryDelayStrategyfield made volatile to support the "caller can invoke setters from any thread" contract.Design notes
Alternative considered: expose a single
setRetryDelayStrategy(RetryDelayStrategy)method that lets the caller replace the whole strategy. Rejected because it gives consumers too much rope — an SDK doing regime switching only needs to move min/max between regimes; it shouldn't be able to accidentally change jitter or backoff-multiplier as part of the same knob. The narrow-setter API keeps the public surface minimal.The
EventSource.baseRetryDelayMillishandling of server-directedretry:hints is orthogonal to the new setters and remains unchanged. A later wire hint continues to overwrite the SDK-set base delay, matching WHATWG HTML Living Standard EventSource semantics.Testing
EventSourceRetryDelayStrategyUsageTest:setInitialRetryDelayMillisUpdatesGetBaseRetryDelayMillissetInitialRetryDelayMillisResetsExponentCountersetMaxRetryDelayMillisClampsAndResetsExponentCountersetInitialAndSetMaxComposeForExtendedRegimeSequence(asserts the RETRY spec's extended-regime doubling shape at ms-scale for test speed)wireRetryHintStillTakesEffectAfterSdkSideSetterssettersOnCustomRetryDelayStrategyDoNotThrowmake contract-tests(sse-contract-tests harness): "All tests passed" end-to-end.Test plan for reviewers
currentRetryDelayStrategybeing volatile is acceptable (the field was previously non-volatile with a comment noting it should only be accessed from the reading thread; the setters lift that invariant, and the volatile write is a defensive publication guarantee).DefaultRetryDelayStrategyare appropriately scoped.wireRetryHintStillTakesEffectAfterSdkSideSetters) — the wire hint continues to override the SDK's initial delay after both setters have been used, which is intentional (SDK regime state ≠ wire-authoritative reconnect time).Downstream
Consumed by java-server-sdk PR (SDK-2789) — Java SDK's RETRY-conformance work. That PR's CI will be red until this PR ships to Maven Central.